-
Notifications
You must be signed in to change notification settings - Fork 228
fix: Update database URL in config and migration logic #2298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi @Abhishek-Punhani. Thanks for your PR. I'm waiting for a kubestellar member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes database connectivity issues by updating the default database name from authdb to authdbui and refactoring the migration logic to use the centralized config package instead of directly accessing environment variables.
Key Changes:
- Updated default PostgreSQL database name in config to match Docker Compose configuration
- Refactored migration logic to use
config.LoadConfig()instead ofos.Getenv()for consistency
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/pkg/config/config.go | Updated default database URL to use authdbui instead of authdb to align with Docker Compose setup |
| backend/postgresql/migrate.go | Replaced direct os.Getenv() calls with config.LoadConfig() for centralized configuration management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok-to-test |
Signed-off-by: Abhishek-Punhani <[email protected]>
Signed-off-by: Abhishek-Punhani <[email protected]>
Signed-off-by: Abhishek-Punhani <[email protected]>
51d8d6f to
8c158f4
Compare
|
/retest-required |
|
Tests are passing now. Ig we can merge this one now? |
|
@btwshivam we can merge this ! |
|
LGTM label has been added. DetailsGit tree hash: e65d0ed0a3c71c5ec0b0a2157567ef89af3956f7 |
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: btwshivam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
However, the default config values in
config.gowere not changed, which is causing errors during the default backend startup.migrate.go, the PostgreSQL URL was directly fetched from os.env, which should have been from the config file to handle the default env configurations in case env vars are not setRelated Issue
Fixes #2297
Changes Made
config.go,migrate.goChecklist
Please ensure the following before submitting your PR:
Screenshots or Logs (if applicable)
Additional Notes